Thanks for the PR! Yes, we could use OPENSSL_cleanse when available.

A couple of minor things to make our CI checks pass:

• can you fix the indenting of the new line?
• can you add a DCO sign-off? See https://github.com/src-d/guide/blob/master/developer-community/fix-DCO.md for instructions on how to amend the commit.

Hi @bencemali, thanks for the PR. It's correct that liboqs could call out to the OPENSSL_cleanse when built against OpenSSL. I'm not an expert on OpenSSL inner workings, but it looks to me as if OPENSSL_cleanse is implemented using the same "trick" (to avoid being optimized out) that we currently have in place: https://github.com/openssl/openssl/blob/openssl-3.3.0/crypto/mem_clr.c. On the other hand, memset_s is (as I understand it) guaranteed to not be optimized out by a standard-compliant compiler: no tricks required. I agree that we should probably call out to OPENSSL_cleanse if memset_s is not available, but I'm not sure we should prefer OPENSSL_cleanse over memset_s if the latter is available.

Hi @bencemali, thanks for the PR. It's correct that liboqs could call out to the OPENSSL_cleanse when built against OpenSSL. I'm not an expert on OpenSSL inner workings, but it looks to me as if OPENSSL_cleanse is implemented using the same "trick" (to avoid being optimized out) that we currently have in place: https://github.com/openssl/openssl/blob/openssl-3.3.0/crypto/mem_clr.c. On the other hand, memset_s is (as I understand it) guaranteed to not be optimized out by a standard-compliant compiler: no tricks required. I agree that we should probably call out to OPENSSL_cleanse if memset_s is not available, but I'm not sure we should prefer OPENSSL_cleanse over memset_s if the latter is available.

Following up on this more concretely: could you please rework the logic so that we only use OPENSSL_cleanse if neither memset_s nor explicit_bzero is available (similarly for SecureZeroMemory on Windows).

OpenSSL guarantees that OPENSSL_cleanse always works and is not optimized out, it is an industry-wide used, trusted, and reviewed solution. The motivation behind this pull request is to use that logic inside OpenSSL instead of platform-specific custom stuff implemented in liboqs.
If OPENSSL_cleanse cannot be the first choice when available then we'd prefer to simply close this PR and have a custom patch in place where we use liboqs.

OpenSSL guarantees that OPENSSL_cleanse always works and is not optimized out, it is an industry-wide used, trusted, and reviewed solution.

For our "peace of mind", do you have a pointer documenting this guarantee, @bencemali ? https://www.openssl.org/docs/manmaster/man3/OPENSSL_cleanse.html seems a bit non-committal. What is true is that the openssl code does not contain a single case of memset_s but a myriad uses of memset, including an interesting one here indicating some deliberation as to what function to be sensible in which case. Conceptually, I agree, though, if OQS relies on/is built using openssl it would be prudent to use the same --particularly security-- mechanisms openssl uses.

My two cents:

OpenSSL guarantees that OPENSSL_cleanse always works and is not optimized out, it is an industry-wide used, trusted, and reviewed solution.The motivation behind this pull request is to use that logic inside OpenSSL instead of platform-specific custom stuff implemented in liboqs. If OPENSSL_cleanse cannot be the first choice when available then we'd prefer to simply close this PR and have a custom patch in place where we use liboqs.

I understand that OpenSSL is a ubiquitous, trusted library. However, liboqs doesn't default to wrapping OpenSSL code whenever possible. Notably, we don't use OpenSSL's SHA3 implementation by default.

For our "peace of mind", do you have a pointer documenting this guarantee, @bencemali ? https://www.openssl.org/docs/manmaster/man3/OPENSSL_cleanse.html seems a bit non-committal. What is true is that the openssl code does not contain a single case of memset_s but a myriad uses of memset, including an interesting one here indicating some deliberation as to what function to be sensible in which case. Conceptually, I agree, though, if OQS relies on/is built using openssl it would be prudent to use the same --particularly security-- mechanisms openssl uses.

I agree that we could rely on OpenSSL here and wash our hands of the matter, but to me it seems like a regression: our fallback code is, as far as I can tell, implemented in exactly the same way as OPENSSL_cleanse. (I actually wonder if we copied directly from OpenSSL back in the day?) We've made a conscious decision in the past to prefer memset_s or explicit_bzero over the generic C approach. I was following that precedent above when I suggested to also prefer memset_s etc. over OPENSSL_cleanse. To break with it requires a stronger rationale than "prefer OpenSSL whenever possible" (which we don't do in general).

I'm open to the idea to defaulting to OPENSSL_cleanse whenever possible for simplicity. However, if we prefer OPENSSL_cleanse over memset_s etc., then we should prefer its approach when we're not building with OpenSSL. Hence, we should also remove the logic to rely on SecureZeroMemory / explicit_bzero / memset_s and simply fall back on the generic volatile function pointer code when OpenSSL is not available.

IMO, what we decide here sets precedent for common (especially OpenSSL-related) code decisions across the project, and we should be able to consistently apply whatever rationale we use for future decisions.

However, if we prefer OPENSSL_cleanse over memset_s etc., then we should prefer its approach when we're not building with OpenSSL.

Good points. This indeed then feels like a regression, particularly considering

our fallback code is, as far as I can tell, implemented in exactly the same way as OPENSSL_cleanse.

First, might it be worth while getting the OpenSSLs team's opinion on your "non-OpenSSL" cleansing code?

Second, this of course implies we accepted OPENSSL_cleanse to be a "good" way to do things -- "just" with the drawback that OQS will not benefit automatically from further improvements to this as & if the code in openssl for this routine changes -- which again speaks for using OPENSSL_cleanse.

This argument does not sound as strong, though:

However, liboqs doesn't default to wrapping OpenSSL code whenever possible. Notably, we don't use OpenSSL's SHA3 implementation by default.

Well, there's been a reason for that and I hope it's not "just" what's documented (?):

These default choices have been made in order to optimize the default performance of all algorithms. Changing them implies performance penalties.

This kind of seems to imply OQS values performance over security. This may be intended but should be clearly documented.

For our "peace of mind", do you have a pointer documenting this guarantee, @bencemali ? https://www.openssl.org/docs/manmaster/man3/OPENSSL_cleanse.html seems a bit non-committal. What is true is that the openssl code does not contain a single case of memset_s but a myriad uses of memset, including an interesting one here indicating some deliberation as to what function to be sensible in which case. Conceptually, I agree, though, if OQS relies on/is built using openssl it would be prudent to use the same --particularly security-- mechanisms openssl uses.

OpenSSL API docs about OPENSSL_cleanse:

OPENSSL_cleanse() fills ptr of size len with a string of 0's. Use OPENSSL_cleanse() with care if the memory is a mapping of a file. If the storage controller uses write compression, then it's possible that sensitive tail bytes will survive zeroization because the block of zeros will be compressed. If the storage controller uses wear leveling, then the old sensitive data will not be overwritten; rather, a block of 0's will be written at a new physical location.

As this function is in OpenSSL's publicly available API and says that it will fill the buffer with zeros, I'd assume it's a good enough that it works. If it would not, it would be treated as a large security issue within OpenSSL itself.

As this function is in OpenSSL's publicly available API and says that it will fill the buffer with zeros, I'd assume it's a good enough that it works. If it would not, it would be treated as a large security issue within OpenSSL itself.

ACK. It's also the reason why OQS apparently also bases its implementation on this as per the statements above.

So we're down to deciding the question by @SWilson4 :

I'm open to the idea to defaulting to OPENSSL_cleanse whenever possible for simplicity. However, if we prefer OPENSSL_cleanse over memset_s etc., then we should prefer its approach when we're not building with OpenSSL. Hence, we should also remove the logic to rely on SecureZeroMemory / explicit_bzero / memset_s and simply fall back on the generic volatile function pointer code when OpenSSL is not available.

IMO, what we decide here sets precedent for common (especially OpenSSL-related) code decisions across the project, and we should be able to consistently apply whatever rationale we use for future decisions.

--> Do we want to follow the OpenSSL approach throughout? Who's going to change the code "in the rest of" liboqs accordingly? I'd argue that it should be the OQS team (by way of a new issue) for the non-OpenSSL code path as this PR does it correctly for the case with OpenSSL used. Opinions, @dstebila @SWilson4 ? We could also label such issue as a non-urgent, "non-product" issue assuming we decide at some point that OQS must only be used in productive settings with OQS_USE_OPENSSL set to ON.

Assuming this is the way we go, I approve.

Do we want to follow the OpenSSL approach throughout?

Just because we use OpenSSL for some things, doesn't mean we have to use them for everything nor agree with every choice they make. As far as I know, there are good reasons to use the dedicated functions SecureZeroMemory / explicit_bzero / memset_s when available.

Who's going to change the code "in the rest of" liboqs accordingly?

I don't understand what the "rest of" is referring to here.

If this is an issue with no clear "best" solution, it could be added as a choice for users. There is an option whether to use OpenSSL for SHA3 called OQS_USE_SHA3_OPENSSL, this could be similar if there would be an OQS_USE_CLEANSE_OPENSSL option as well.

it could be added as a choice for users.

Doing this would a) create more (too much?) configurability and b) make us shirk a decision on a good default -- that I think a trustworthy security software library should be able to make for its users -- that by and large (should be allowed to) know less than the developers of the library.

As far as I know, there are good reasons to use the dedicated functions SecureZeroMemory / explicit_bzero / memset_s when available.

Can you please elaborate on them & particularly why they are apparently not used by OpenSSL?

I don't understand what the "rest of" is referring to here.

I meant all code requiring secure memory clearing in the absence of OpenSSL. If there is none or if the macro is already used throughout the code base, I agree this may be the empty set.

it could be added as a choice for users.

Doing this would a) create more (too much?) configurability and b) make us shirk a decision on a good default -- that I think a trustworthy security software library should be able to make for its users -- that by and large (should be allowed to) know less than the developers of the library.

I agree, I don't think this level of configurability is necessary.

As far as I know, there are good reasons to use the dedicated functions SecureZeroMemory / explicit_bzero / memset_s when available.

Can you please elaborate on them & particularly why they are apparently not used by OpenSSL?

For these specific functions, the compilers promise that they will not optimize the call away. As far as I know, the volatile trick that we use as a backup (and that OpenSSL uses as a default) is just that -- a trick, which we think that compilers will not optimize away, but there's no formal guarantee that compilers won't optimize it away.

I don't understand what the "rest of" is referring to here.

I meant all code requiring secure memory clearing in the absence of OpenSSL. If there is none or if the macro is already used throughout the code base, I agree this may be the empty set.

This should be the only place that the change is needed in our code base -- the rest of the code in liboqs is meant to be calling OQS_MEM_cleanse. We have some CI tests complaining about bare free calls (instead of calls to OQS_MEM_secure_free or OQS_MEM_insecure_free). Although I don't think we have any CI tests scanning for memset's, which I speculate would have many false positives.

This should be the only place that the change is needed in our code base

Thanks for that assessment. In that case, I'd suggest accepting the PR as-is and consider the issue sufficiently dealt with. OK, @dstebila @SWilson4 ?

OK, I just took a closer look through the OpenSSL repo and found what appear to be assembly implementations of OPENSSL_cleanse. I had missed these originally because they're contained in Perl scripts. (I assume that the scripts are used to generate code somehow; I'm not familiar with the OpenSSL build process.)

With this knowledge, I'm OK with this PR as-is. What I said above about it being a regression does not apply, as the OpenSSL implementation apparently does not simply default to the approach that we use as a fallback.

I'll approve once CI is green (a rebase on main will hopefully clear up the failing style check). Sorry for holding this up: I should have figured that OpenSSL would have assembly implementations for something as basic (and easily optimized out) as zeroing memory.

This should be the only place that the change is needed in our code base

Thanks for that assessment. In that case, I'd suggest accepting the PR as-is and consider the issue sufficiently dealt with. OK, @dstebila @SWilson4 ?

Okay with me.

Okay with me.

Thanks @dstebila .

@bencemali Please note the CI failure: It seems to be relevant.

Thanks @bencemali for the fix in the latest commit! CI passes. Now a second review/approval please @SWilson4 @dstebila so we can merge this finally.

Thanks for the changes @bencemali!